Skip to content

feat(git): go-git v6, so Azure DevOps is now also supported - #297

Merged
sunib merged 7 commits into
mainfrom
feat/go-git-v6
Jul 30, 2026
Merged

feat(git): go-git v6, so Azure DevOps is now also supported#297
sunib merged 7 commits into
mainfrom
feat/go-git-v6

Conversation

@sunib

@sunib sunib commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Pull Request

Fixes #288 by moving to go-git v6 (v6.0.0-alpha.5), which implements the multi_ack capability
Azure DevOps insists on. Alternative to #292, which reaches for a system git binary instead; once
go-git#1204 landed, upstream deleted their own ADO workaround example saying it "works out of the box".

multi_ack is in every v6 tag, and the churn in the fourteen packages we import has settled — 96 → 39
19 exported removals per alpha, the one breaking wave being the transport rewrite in
spring. v5 is still released alongside (v5.19.2 on alpha.5's day), so backing out stays possible.

What v6 changes

The API moves were mechanical. The interesting part is that v6 reads ambient system state that v5
ignored, and fails closed
— twice, in places no unit test can reach.

v5 v6
transport.AuthMethod one interface gone; auth is []client.Option — closures, so not inspectable
push session NewReceivePackSessionAdvertisedReferencesReceivePack HandshakeGetRemoteRefsPush, same *packp.Command
commit.gpgSign ignored consulted when Signer is nil; refuses the commit if set with no signer
HostKeyAlgorithms never derived derived from on-disk known_hosts even when a callback is set; hard-fails if absent
file:// transport spawned the real git-receive-pack in-process server that never compares cmd.Old

The one that would have shipped broken. v6 loads ~/.ssh/known_hosts and
/etc/ssh/ssh_known_hosts to derive HostKeyAlgorithms whenever ClientConfig returns it empty —
including when we already supplied a HostKeyCallback — and fails the connection when neither file
exists. Our image is distroless with neither, so every SSH remote would have failed regardless of the
credential
. ssh.KeyAuth now always populates the list: from the pinned known_hosts when there is
one, a modern default set otherwise.

Unit tests cannot see this. The fallback lives in the transport's connect, not in ClientConfig, so
building a credential and asserting on it passes cleanly — two of mine did. The e2e SSH spec against a
real Gitea is what caught it.

commit.gpgSign is the same shape: any machine or image with it set globally breaks every commit we
make. PinExplicitSigningPolicy writes the repo-local value false, so our signing policy comes from
the GitProvider rather than from whatever gitconfig the process can see.

file:// stopped being a real server, which silently weakened a test. v6's in-process
receive-pack only checks that a ref exists and then sets it — it never compares cmd.Old — so racing
pushes all win, and TestBranchWorker_ConcurrentOperations was passing with 2 commits where it asserts
4. It now runs against a real git server, and anything asserting the compare-and-swap must avoid
file://. Probably worth reporting upstream.

The atomic push itself is unchanged: one session still serves both the advertisement and the push, and
PushRequest.Commands takes the same *packp.Command, so the server-side Old/New compare-and-swap
is verbatim. v6 also negotiates report-status itself and returns a rejected command as the error from
Push, collapsing our separate status inspection into one check, and Atomic is now a first-class
field.

The test, without an Azure DevOps tenant

Nobody here has an ADO org, so the failure is reproduced locally. Canonical git's own upload-pack
advertises multi_ack, which makes git-http-backend behind a proxy enforcing ADO's rule a faithful
simulator for both halves: the proxy reproduces the rejected request, the real backend reproduces the
multi-ACK response v5 also cannot parse. The v2 opt-in header is stripped so a v2-capable client cannot
sidestep the capability under test.

test v5 v6
TestADOSimulator_IsFaithful pass pass
TestADO_CheckRepo_NeedsNoNegotiation pass pass
TestADO_PushAtomic_NeedsNoMultiAck pass pass
TestADO_SmartFetch_RequiresMultiAck fail — HTTP 400 TF401041 pass

The two middle ones passing on v5 locate the bug: only repo.Fetch was ever affected. CheckRepo
reads just the ref advertisement, and receive-pack has no multi_ack at all.

Background and the options considered:
docs/design/azure-devops-multi-ack.md.

I also tested it live with a PAT and it works, and even attribution is shown nicely in ADO as well:

chrome_U2jeAtcad1

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Refactoring — the auth abstraction
  • Test coverage improvement

Testing

  • task lint
  • task test
  • task test-e2e (default leg): 79 passed, 22 skipped, 0 failed
  • task test-image-refresh locally: 6 passed, 0 failed — the CI shard's first failure was a
    bring-up flake (apiserver EOF during flux-operator install, zero Ginkgo reports, no spec ran)

Related Issues

Closes #288
Alternative to #292

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added an Azure DevOps–focused setup guide with PAT (username optional) and improved branch/repo configuration guidance.
    • Added bearer-token credential support and ADO-specific e2e coverage (including a dedicated test task/labeling).
  • Bug Fixes

    • Improved Azure DevOps fetch compatibility by enforcing multi-ack requirements.
    • Hardened SSH credential handling and known-hosts verification behavior.
    • Pinned explicit commit signing policy to avoid ambient GPG signing.
  • Documentation

    • Updated Azure DevOps configuration/security-model and multi-ack troubleshooting docs.
  • Tests

    • Added simulator, live canary, and additional e2e tests for multi-ack behavior.

Azure DevOps rejects any protocol-v0 upload-pack request whose capability
list omits multi_ack, with HTTP 400 "TF401041: Clients must support
multi-ack." go-git v5 keeps MultiACK and MultiACKDetailed in
transport.UnsupportedCapabilities and deletes them from the server's
advertisement as it parses, so the capability is never requested and every
fetch against ADO fails (#288).

v6 implements the capability (go-git#1204), and upstream then deleted their
own ADO workaround example saying it "works out of the box". This takes
v6.0.0-alpha.5 -- the latest tag, and identical to upstream main -- rather
than PR #292's fallback to a bundled system git binary, which measured at
+723 MB of image and left the CRITICAL image-scan gate blind to git,
OpenSSH and OpenSSL because they arrive as loose files with no package
database. The reasoning, the measurements and the four options are in
docs/design/azure-devops-multi-ack.md.

The blast radius of the ADO problem is one call, repo.Fetch. CheckRepo and
listRemoteRefs read only the ref advertisement, and PushAtomic speaks
receive-pack, which has no multi_ack at all. The tests assert exactly that,
and they passed on v5.

Red-first, and it needs no Azure DevOps tenant: canonical git's own
upload-pack advertises multi_ack, so git-http-backend behind a proxy that
enforces ADO's rule is a faithful simulator for both halves -- the proxy
reproduces the rejected request, the real backend reproduces the multi-ACK
response v5 also cannot parse. The v2 opt-in header is stripped so a
v2-capable client cannot sidestep the capability under test.
TestADO_SmartFetch_RequiresMultiAck fails on v5 with ADO's exact 400 and
passes on v6.

The API migration:

  - transport.AuthMethod is gone; auth is functional options. A credential
    now travels as []gitclient.Option, and git.Credential keeps the
    concrete value alongside so the Secret-key-to-auth-field mapping stays
    assertable -- the options are closures and cannot be inspected.
  - transport.NewEndpoint + client.NewClient + NewReceivePackSession
    become transport.ParseURL + gitclient.New(opts).Handshake.
  - AdvertisedReferences becomes GetRemoteRefs, returning a slice.
  - ReceivePack becomes Session.Push.

The atomic push keeps its guarantee unchanged: one session serves both the
advertisement and the push, and PushRequest.Commands takes the same
*packp.Command, so the server-side Old/New compare-and-swap is verbatim.
v6 negotiates report-status itself and returns a rejected command as the
error from Push, so the separate status inspection collapses into one
check, and Atomic is now a first-class field.

Two settings v6 reads from the environment that v5 ignored, both failing
closed, and neither visible to a unit test:

  - commit.gpgSign, merged across system, global and local scope, is
    consulted whenever CommitOptions.Signer is nil, and refuses the commit
    when set with no signer registered. Any host or image with it set
    would break every commit we make. PinExplicitSigningPolicy writes the
    local value false at init: our signing policy comes from the
    GitProvider, not from ambient config.
  - HostKeyAlgorithms is derived by reading ~/.ssh/known_hosts and
    /etc/ssh/ssh_known_hosts whenever ClientConfig returns it empty, even
    when a HostKeyCallback was supplied, and hard-fails when neither file
    exists. The controller image is distroless with neither, so every SSH
    remote would have failed in production regardless of the credential.
    ssh.KeyAuth now always populates the list, from the pinned known_hosts
    when there is one and a modern default set otherwise. Found by the e2e
    suite against a real Gitea; no unit test can reach it, because the
    fallback lives in the transport's connect rather than in ClientConfig.

TestBranchWorker_ConcurrentOperations moves to a real git server. v5's
file:// transport spawned the real git-receive-pack; v6 runs go-git's
in-process one, whose updateReferences never compares cmd.Old and just
sets the reference, so over file:// every racing push wins and the test
was passing vacuously with 2 commits instead of 4. Any future test of the
compare-and-swap must avoid file://.

Closes #288
Refs #292

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR migrates Git handling from go-git v5 to v6, introduces structured credential and SSH authentication handling, updates fetch and push transport flows, adds Azure DevOps multi-ACK simulator and live/e2e coverage, and documents the migration decision and setup.

Changes

Git transport migration

Layer / File(s) Summary
Dependency and authentication contracts
go.mod, internal/git/credentials.go, internal/ssh/auth.go, internal/controller/*
Go-git v6 client options replace v5 transport auth methods; credentials support SSH, basic, bearer, and anonymous forms.
Fetch and atomic push transport flow
internal/git/git_smart_fetch.go, internal/git/git_atomic_push.go, internal/git/git.go
Fetch and push use v6 client options and session APIs, including multi-ACK fetch negotiation and atomic push requests.
Azure DevOps regression and integration coverage
internal/git/ado_multiack_test.go, internal/git/ado_live_test.go, test/e2e/*
Tests simulate Azure DevOps upload-pack behavior and validate connectivity, pushes, fetches, branch resolution, and resource replication.
Repository and test compatibility updates
internal/git/*, internal/manifestanalyzer/*
Go-git imports, filesystem accessors, signing APIs, repository initialization, and related tests are updated for v6.
Design and setup documentation
docs/design/azure-devops-multi-ack.md, docs/azure-devops-getting-started.md, docs/configuration.md, docs/INDEX.md
Documents the multi-ACK issue, migration decision, Azure DevOps configuration, testing approach, and implementation findings.

Estimated code review effort: 4 (Complex) | ~60 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR upgrades to go-git v6 and adds regressions/tests that address Azure DevOps multi_ack fetch failures from #288.
Out of Scope Changes check ✅ Passed The added docs, auth/signing fixes, and test updates all support the go-git v6/Azure DevOps migration and are in scope.
Docstring Coverage ✅ Passed Docstring coverage is 84.73% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The title accurately summarizes the main change: upgrading to go-git v6 to enable Azure DevOps support.
Description check ✅ Passed The description covers the change summary, type, testing, related issues, and extra notes, though the checklist section is missing.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/go-git-v6

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (5)
internal/controller/ssh_test.go (1)

349-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Name the split-out test after the function under test.

The subject is extractCredential, so TestExtractCredential_HTTPAndAnonymous matches the convention; TestCredentials_... refers to no existing function.

As per coding guidelines: "name tests TestFunctionName_Scenario(t *testing.T)".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/ssh_test.go` around lines 349 - 352, Rename the split-out
test function from TestCredentials_HTTPAndAnonymous to
TestExtractCredential_HTTPAndAnonymous so its name follows the
function-under-test convention for extractCredential while preserving the
existing test behavior.

Source: Coding guidelines

internal/controller/gitprovider_controller_test.go (1)

59-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

These two passphrase specs are now identical and neither exercises a passphrase.

Both blocks supply the same unencrypted key plus ssh-passphrase: "", and sshPassphrase in internal/git/credentials.go only reads ssh-password/password — so ssh-passphrase is never consulted and both specs reduce to the no-passphrase case already covered at Lines 39-57. Worth either dropping one and pointing the other at a recognized key, or asserting something passphrase-specific.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/gitprovider_controller_test.go` around lines 59 - 114,
Remove the duplicate empty-passphrase coverage in the SSH credential specs
around generateTestSSHKey and extractCredential, or replace one test with a
recognized ssh-password/password key and an encrypted SSH key. Ensure the
remaining test exercises behavior specific to the supported passphrase lookup
rather than repeating the unencrypted no-passphrase case.
internal/git/credentials_test.go (2)

91-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

x, ok := v, v != nil is a leftover of the removed type assertion.

Now that Credential exposes typed fields, require.NotNil reads better and the extra ok variable disappears.

♻️ Proposed simplification
-		basic, ok := auth.Basic, auth.Basic != nil
-		require.True(t, ok)
-		assert.Equal(t, "u", basic.Username)
-		assert.Equal(t, "p", basic.Password)
+		require.NotNil(t, auth.Basic)
+		assert.Equal(t, "u", auth.Basic.Username)
+		assert.Equal(t, "p", auth.Basic.Password)
-		token, ok := auth.Bearer, auth.Bearer != nil
-		require.True(t, ok)
-		assert.Equal(t, "gho_token", token.Token)
+		require.NotNil(t, auth.Bearer)
+		assert.Equal(t, "gho_token", auth.Bearer.Token)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/git/credentials_test.go` around lines 91 - 108, In the Basic and
bearer token test cases, replace the `x, ok := v, v != nil` patterns with direct
typed-field variables and `require.NotNil` assertions. Remove the redundant `ok`
variables while preserving the existing username, password, and token
assertions.

50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Test names still reference the pre-migration functions.

These now exercise CredentialFromSecretData / credentialFromSecret, so the TestAuthFromSecretData_* and TestGetAuthFromSecret_* names point at the thin wrappers rather than the code under test.

As per coding guidelines: "name tests TestFunctionName_Scenario(t *testing.T)".

Also applies to: 69-69, 83-83, 247-247

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/git/credentials_test.go` at line 50, Rename the affected tests in
internal/git/credentials_test.go to follow the functions they directly exercise:
replace the outdated TestAuthFromSecretData_* and TestGetAuthFromSecret_*
prefixes with CredentialFromSecretData and credentialFromSecret respectively,
while preserving each scenario suffix.

Source: Coding guidelines

internal/git/git_atomic_push.go (1)

198-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the exported v6 API changes.

Both updated exported functions lack Go doc comments.

  • internal/git/git_atomic_push.go#L198-L203: add a PushAtomic comment describing its compare-and-swap push contract and client-option authentication.
  • internal/git/git_smart_fetch.go#L25-L30: add a SmartFetch comment describing its fetch/result behavior and client-option authentication.

As per coding guidelines, “add godoc comments for all exported identifiers.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/git/git_atomic_push.go` around lines 198 - 203, Add Go doc comments
for the exported functions PushAtomic in internal/git/git_atomic_push.go (lines
198-203) and SmartFetch in internal/git/git_smart_fetch.go (lines 25-30).
Document PushAtomic’s compare-and-swap push contract and client-option
authentication, and document SmartFetch’s fetch/result behavior and
client-option authentication.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/INDEX.md`:
- Line 88: Update the Azure DevOps multi-ACK entry in the documentation index to
reflect that Option A is decided and implemented rather than decision needed.
Also increment the “Sixteen other open items” count to seventeen, preserving the
rest of the entry’s content.

In `@internal/git/git_atomic_push.go`:
- Line 40: In the atomic-push flow around transport.ParseURL, validate that
remote.Config().URLs contains at least one entry before accessing URLs[0].
Return the existing reconciliation error type or path for a configured origin
with no URLs, and add a negative test covering this empty-URLs case without
changing behavior for valid remotes.

In `@internal/git/git.go`:
- Around line 640-642: Update PrepareBranch so PinExplicitSigningPolicy(repo)
runs after both repository acquisition paths, including when tryOpenExistingRepo
returns an existing checkout, while preserving error propagation. Add a
regression test covering an already-existing checkout with ambient
commit.gpgSign=true and verifying unsigned commits proceed successfully.

In `@internal/ssh/auth_test.go`:
- Around line 142-152: Strengthen the “with a pinned known_hosts” subtest by
asserting cfg.HostKeyAlgorithms differs from defaultHostKeyAlgorithms(), proving
the pin lookup supplied the algorithms rather than fallback. Also cover a
non-22-port request if supported by req to validate the hostWithPort(req) lookup
key.

---

Nitpick comments:
In `@internal/controller/gitprovider_controller_test.go`:
- Around line 59-114: Remove the duplicate empty-passphrase coverage in the SSH
credential specs around generateTestSSHKey and extractCredential, or replace one
test with a recognized ssh-password/password key and an encrypted SSH key.
Ensure the remaining test exercises behavior specific to the supported
passphrase lookup rather than repeating the unencrypted no-passphrase case.

In `@internal/controller/ssh_test.go`:
- Around line 349-352: Rename the split-out test function from
TestCredentials_HTTPAndAnonymous to TestExtractCredential_HTTPAndAnonymous so
its name follows the function-under-test convention for extractCredential while
preserving the existing test behavior.

In `@internal/git/credentials_test.go`:
- Around line 91-108: In the Basic and bearer token test cases, replace the `x,
ok := v, v != nil` patterns with direct typed-field variables and
`require.NotNil` assertions. Remove the redundant `ok` variables while
preserving the existing username, password, and token assertions.
- Line 50: Rename the affected tests in internal/git/credentials_test.go to
follow the functions they directly exercise: replace the outdated
TestAuthFromSecretData_* and TestGetAuthFromSecret_* prefixes with
CredentialFromSecretData and credentialFromSecret respectively, while preserving
each scenario suffix.

In `@internal/git/git_atomic_push.go`:
- Around line 198-203: Add Go doc comments for the exported functions PushAtomic
in internal/git/git_atomic_push.go (lines 198-203) and SmartFetch in
internal/git/git_smart_fetch.go (lines 25-30). Document PushAtomic’s
compare-and-swap push contract and client-option authentication, and document
SmartFetch’s fetch/result behavior and client-option authentication.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f7284c3a-49bf-4bf7-8920-5974222c51d9

📥 Commits

Reviewing files that changed from the base of the PR and between b5d15d9 and 8d79a78.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (53)
  • docs/INDEX.md
  • docs/design/azure-devops-multi-ack.md
  • go.mod
  • internal/controller/gitprovider_controller.go
  • internal/controller/gitprovider_controller_test.go
  • internal/controller/ssh_test.go
  • internal/git/acceptance_gate_test.go
  • internal/git/ado_multiack_test.go
  • internal/git/bootstrapped_repo_template.go
  • internal/git/branch_worker.go
  • internal/git/branch_worker_metrics_test.go
  • internal/git/branch_worker_split_test.go
  • internal/git/branch_worker_test.go
  • internal/git/commit.go
  • internal/git/commit_executor.go
  • internal/git/commit_executor_test.go
  • internal/git/commit_request_attach_test.go
  • internal/git/credentials.go
  • internal/git/credentials_test.go
  • internal/git/fieldpatch_flush_test.go
  • internal/git/git.go
  • internal/git/git_atomic_push.go
  • internal/git/git_atomic_push_test.go
  • internal/git/git_operations_test.go
  • internal/git/git_smart_fetch.go
  • internal/git/helpers.go
  • internal/git/helpers_test.go
  • internal/git/inplace_edit_test.go
  • internal/git/inplace_overrides_test.go
  • internal/git/known_placement_bugs_test.go
  • internal/git/kustomize_delete_test.go
  • internal/git/kustomize_oracle_test.go
  • internal/git/patches_test.go
  • internal/git/placement_metrics_test.go
  • internal/git/placement_test.go
  • internal/git/plan_flush.go
  • internal/git/plan_flush_test.go
  • internal/git/prune_mode_test.go
  • internal/git/render_fidelity_test.go
  • internal/git/render_scope_test.go
  • internal/git/resync_flush.go
  • internal/git/resync_flush_test.go
  • internal/git/resync_heal_test.go
  • internal/git/resync_push_test.go
  • internal/git/secret_write_test.go
  • internal/git/signing.go
  • internal/git/signing_test.go
  • internal/git/source_form_test.go
  • internal/git/types.go
  • internal/git/write_boundary_precondition_test.go
  • internal/manifestanalyzer/gittargetignore.go
  • internal/ssh/auth.go
  • internal/ssh/auth_test.go

Comment thread docs/INDEX.md Outdated
Comment thread internal/git/git_atomic_push.go Outdated
Comment thread internal/git/git.go Outdated
Comment thread internal/ssh/auth_test.go
@sunib

sunib commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

e2e green

task test-e2e after the SSH HostKeyAlgorithms fix, read from the Ginkgo JSON report rather than the log:

passed: 79  |  skipped: 22  |  failed: 0
Ran 71 of 93 Specs in 706.683 seconds — SUCCESS!

should validate GitProvider with SSH authenticationpassed (18s). That is the spec that caught the regression: on the previous run it timed out after 90s with the GitProvider stuck at Ready=False and the controller logging

unable to find any valid known_hosts file, set SSH_KNOWN_HOSTS env variable

against a real in-cluster Gitea over ssh://. Worth restating why unit tests could not have found it: go-git v6 derives HostKeyAlgorithms from the process's default known_hosts files inside the transport's connect, not in ClientConfig, and does so even when a HostKeyCallback was supplied. A test that builds a credential and inspects it passes cleanly — two of mine did — because the fallback is one layer below where the credential is visible. The image is distroless with no home directory and no /etc/ssh/ssh_known_hosts, so every SSH remote would have failed in production regardless of the credential.

The regression test asserts the property that matters (the algorithm list is never empty, under both host-key policies) rather than re-asserting credential shape.

Caveat worth carrying into review

TestBranchWorker_ConcurrentOperations only surfaced the file:// compare-and-swap problem because it counts commits — it expected 4 and got 2. Other tests that push over file:// would not notice the Old/New check going unenforced, so I can't claim nothing else weakened silently; I found the one test that happened to assert a count. That is the argument for the startRealGitServer helper being the default for anything asserting push rejection.

Full status: task lint, task test and task test-e2e all green on b5d15d98 + this commit.

@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.96732% with 23 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/git/credentials.go 83.3% 6 Missing and 1 partial ⚠️
internal/git/git.go 56.2% 3 Missing and 4 partials ⚠️
internal/ssh/auth.go 85.3% 3 Missing and 3 partials ⚠️
internal/git/git_atomic_push.go 90.6% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

…address the review

Four review findings, all real:

  - PrepareBranch pinned the signing policy only on repositories it CREATED.
    Worker clones live on a volume across restarts, and a repository made
    before the pin existed is the common case on upgrade -- either would hit
    go-git v6's "cannot auto-sign commit" the first time an ambient
    commit.gpgSign is true. The pin now covers the reuse path too, which is
    where it matters most.
  - getPushSession indexed remote.Config().URLs[0] unguarded. go-git rejects
    a URL-less remote in its own validation, but a hand-edited .git/config
    can still present one, and indexing it panics rather than fails.
  - The pinned-known_hosts subtest could not tell a pin hit from the default
    fallback, because the default set also carries the RSA algorithms. It now
    asserts the list is narrower than the default and excludes ed25519, which
    only the pin can produce.
  - docs/INDEX.md still said "decision needed" after the design record moved
    to decided-and-built, and undercounted its own list by one.

Separately, and found by porting #292's Azure DevOps examples: the credential
those examples document did not work. ADO sends a PAT as HTTP basic auth with
the token as the password and ignores the username, so its documented Secret
carries an empty username -- and firstSecretValue treats an empty value as an
absent key, so the basic-auth branch never fired and the Secret was refused
with "does not contain valid authentication data". Pre-existing on v5 too.
The password is what carries the credential, so it is what we branch on now. A
username with no password stays an error, because that one is a real mistake.

Verified against a real Azure DevOps repository, which is what the new opt-in
tests are for. Both skip themselves without a credential, so CI is unchanged:

  - internal/git/ado_live_test.go walks the branch-resolution contract against
    the real remote in the order the code implements it -- empty repository
    resolves to nothing, an absent target falls back to the default, a present
    target wins while the default is still fetched as a safety net -- and ends
    on the negotiating fetch, the one request go-git v5 cannot make.
  - test/e2e/ado_e2e_test.go proves the operator mirrors a live ConfigMap into
    a real ADO repository. It reads the result back with canonical git rather
    than our own library, so the assertion does not depend on the code under
    test.

docs/azure-devops-getting-started.md is written from that e2e recipe, and says
plainly why the connectivity check can pass on a release where every fetch
fails: the advertisement is a different request that never needed multi_ack.

Refs #288

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib

sunib commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Pushed e67adb8: all four inline review comments fixed, plus a bug the ported examples surfaced.

The four review findings

All valid. The signing-policy one was the most consequential — the pin covered only repositories PrepareBranch creates, not the ones it reuses, and reuse is the common case: worker clones live on a volume across restarts, and any repository created before the pin existed arrives that way on upgrade. Either would have hit v6's cannot auto-sign commit on the first commit under an ambient commit.gpgSign. Now pinned on both paths, with a test that sets the setting on an existing repository and asserts the next PrepareBranch re-pins it.

The HostKeyAlgorithms subtest was genuinely vacuous, as flagged: the default set also carries the RSA algorithms. The pin measurably yields [rsa-sha2-512 rsa-sha2-256 ssh-rsa], so it now asserts the list is narrower than the default and excludes ed25519.

A bug the Azure DevOps examples surfaced

Porting the ADO examples from #292 found that the credential those examples document did not work. ADO sends a PAT as HTTP basic auth with the token as the password and ignores the username, so its documented Secret carries an empty username — and firstSecretValue treats an empty value as an absent key, so the basic-auth branch never fired and the Secret was refused with "does not contain valid authentication data". Pre-existing on v5, so #292 would have shipped docs that its own code rejects.

The password is what carries the credential, so it is what we branch on now. A username with no password stays an error.

Verified against real Azure DevOps

@sunib supplied a scratch ADO repository, so the belief the local simulator encodes is now checked against the real thing. Two opt-in layers, both skipping themselves without a credential, so CI is unchanged:

  • internal/git/ado_live_test.go walks the branch-resolution contract in the order the code implements it, against the real remote:

    phase 1: default branch is "main" at 9ab745e0 (unborn=false)
    phase 2: target "…-absent-…" is absent, resolved to "main"
    phase 3: target "…-feature-…" exists, resolved to "…-feature-…"   ← target wins
             (origin/main still fetched — the safety net)
    phase 4: negotiated fetch advanced origin/main to a2ca75d2       ← v5 dies here with TF401041
    
  • test/e2e/ado_e2e_test.go proves the operator mirrors a live ConfigMap into a real ADO repository — passed. It reads the result back with canonical git rather than our own library, so the assertion does not depend on the code under test. Labelled ado and excluded from the default suite; task test-e2e-ado runs it.

docs/azure-devops-getting-started.md is written from that e2e recipe, and spells out the thing that makes this confusing to debug: the connectivity check can pass on a release where every fetch fails, because the advertisement is a different request that never needed multi_ack.

On the "Out of Scope Changes" warning

I would push back on splitting these out. The signing pin and the SSH HostKeyAlgorithms fix are not opportunistic hardening — they are v6 migration fallout, and without them this PR is broken: every commit fails wherever commit.gpgSign is set, and every SSH remote fails in the distroless image. The file:// test change is the same story; that test fails on v6 otherwise, because go-git's in-process receive-pack never compares cmd.Old. Landing the upgrade without them would mean landing a known-broken upgrade.

… at a time

I probed ADO with a bare `want <sha>` carrying no capability list, got HTTP
200, and concluded that the six-year-old bug reports were wrong. They are not.
That shape is the one row ADO accepts, and no real client sends it. Running
go-git v5.19.1 against the same repository reproduced the reported failure
immediately -- and not on a fetch, on the CLONE:

  CLONE FAILED: unexpected client error: unexpected requesting
  ".../git-upload-pack" status code: 400

The rule, measured one request shape at a time against a real repository:

  want <sha>                                        -> 200
  want <sha> side-band-64k ofs-delta agent=...      -> 400 TF401041
  want <sha> side-band-64k                          -> 400 TF401041
  want <sha> agent=git/2.39.5                       -> 400 TF401041
  want <sha> multi_ack side-band-64k ofs-delta      -> 200
  want <sha> multi_ack_detailed side-band-64k       -> 200

So the trigger is a capability list that omits multi_ack, not the absence of
negotiation, and either capability satisfies it. v5 filters both out of the
advertisement before deciding what to ask for, so every request it sends is
the rejected shape.

The doc keeps the wrong turn rather than quietly correcting it, because the
failure mode is worth naming: probing with a hand-rolled request tests the
request you built, not the behaviour you are attributing to it, and
contradicting a long-standing external bug report needs a reproduction with
the real client rather than a curl that disagrees.

Also records what canonical git advertises (which is why it never notices this
at all, and why git-http-backend is a usable stand-in), that receive-pack has
no multi_ack whatsoever so pushing was never affected on any version, and the
sources behind each claim.

TestADOLive_StillRequiresMultiAck turns the premise into a canary: it asserts
ADO STILL rejects the capability-list-without-multi_ack shape, and fails loudly
with "GOOD NEWS, NOT A BUG" if Microsoft ever fixes it. Verified against the
real server, which answers:

  TF401041: The Git protocol sent is not as expected (Clients must support
  multi-ack.).

The public-fixture idea is dropped -- Azure DevOps no longer allows new public
projects -- so the live tests stay opt-in and PAT-gated, and never run in CI.
E2E_ADO_EMPTY_REPO_URL names a repository that stays empty, which is the only
way to cover the empty-repository contract more than once: the main fixture
seeds itself on first run.

Refs #288

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/azure-devops-getting-started.md`:
- Around line 144-146: Update the documentation wording around
E2E_ADO_EMPTY_REPO_URL to clarify that the variable should point to a repository
that remains empty, rather than implying the variable itself must be unset or
empty; preserve the explanation that nothing writes to this repository and it
supports repeated empty-repository contract coverage.

In `@docs/facts/azure-devops-multi-ack-requirement.md`:
- Around line 148-150: Update the simulator description to say it rejects any
upload-pack POST without either capability, multi_ack or multi_ack_detailed.
Preserve the explanation that the simulator is stricter than ADO and
intentionally keeps the gating logic simple.

In `@internal/git/ado_live_test.go`:
- Around line 375-386: Bound the raw HTTP request in the canary test by
replacing the background context used around the request with a context carrying
a finite timeout, and ensure that timeout is released appropriately. Apply this
to the request created in the canary flow before http.DefaultClient.Do so
unreachable or hanging Azure DevOps calls terminate cleanly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9100ef7c-7437-4b87-b632-68730b82fea8

📥 Commits

Reviewing files that changed from the base of the PR and between e67adb8 and f7b2c39.

📒 Files selected for processing (3)
  • docs/azure-devops-getting-started.md
  • docs/facts/azure-devops-multi-ack-requirement.md
  • internal/git/ado_live_test.go

Comment thread docs/azure-devops-getting-started.md Outdated
Comment thread docs/facts/azure-devops-multi-ack-requirement.md Outdated
Comment thread internal/git/ado_live_test.go
sunib and others added 4 commits July 30, 2026 15:09
configuration.md had grown a full Azure DevOps walkthrough inline -- the Secret,
the GitProvider, Entra, SSH, and the multi_ack background -- duplicating
azure-devops-getting-started.md a screen below the GitProvider example.
github-setup-guide.md already establishes the pattern: per-provider setup is its
own page, and configuration.md points at it.

So the section goes, and what stays is the single thing that surprises people,
placed where it bites: a note under the credentials-Secret auth-keys table,
which is exactly where a reader learns HTTP basic means username + password and
needs to know Azure DevOps is the exception that carries only a password.

Also wires E2E_ADO_EMPTY_REPO_URL through the getting-started guide, and names
TestADOLive_StillRequiresMultiAck as a canary there, so someone hitting a red
build knows a failure means Microsoft fixed their end rather than that something
broke.

Verified against the real fixtures at dev.azure.com/configbutler/tests: all four
live tests pass, including the empty-repository case against a repository that
stays empty and the canary, which reports

  TF401041: The Git protocol sent is not as expected (Clients must support
  multi-ack.).

and the operator-level e2e spec mirrors a live ConfigMap into the repository
(1 passed, 0 failed).

Refs #288

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cted themselves

Three review findings, all valid:

  - The canary had no deadline on either half: context.Background() and
    http.DefaultClient's zero Timeout. An unreachable or mid-response Azure
    DevOps would hang the test rather than reaching the skip its error path
    already intends. Bounded at 30s.
  - "E2E_ADO_EMPTY_REPO_URL must stay empty" reads as an instruction to unset
    the variable, two lines below the command that sets it. It is the
    REPOSITORY it names that must stay empty.
  - The facts page claimed the simulator "rejects any upload-pack POST without
    multi_ack", which contradicted its own measured table two screens above:
    the check matches the multi_ack PREFIX, so multi_ack_detailed satisfies it,
    exactly as ADO does. Verified rather than assumed -- "multi_ack" is a
    substring of "multi_ack_detailed", and the simulator's test is
    bytes.Contains.

The third is the one worth noticing: a page whose entire purpose is to record
what was measured had a summary sentence disagreeing with its own table.

Refs #288

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback. The guide read as a tour rather than a path a newcomer can
follow, and advertised two credential types nobody has tested against Azure
DevOps.

  - SSH and Entra bearer tokens are no longer presented as setup steps. They
    use the same Secret keys as any other provider and probably work exactly
    as they do for GitHub, but nothing here has exercised them against ADO, so
    they are named once and labelled untested rather than walked through.
  - `kubectl create namespace` is now in the flow. The Secret command failed
    without it.
  - The branch is a `<branch>` placeholder used in both resources, instead of
    a hard-coded `main` the reader had no reason to think was a choice.
  - The PAT step says which organization, to set an expiry, to copy the token
    when shown, and that it inherits its user's repository permissions.
  - security-model.md claimed HTTP basic auth requires `username`, which
    contradicts the Azure PAT contract this branch introduced. It now says the
    password is what selects basic auth, and that ADO omits the username.

Both pages are shorter: 155 -> 125 lines for the guide, and the facts page
loses the essay while keeping the measurements and sources.

One review point is not applied, because the evidence contradicts it: audit
delivery is NOT a prerequisite for the final ConfigMap to produce a commit.
Attribution is what needs audit; writes do not. The chart defaults to
configured-author with attribution disabled, and test/e2e/ado_e2e_test.go sets
up no audit at all yet passes by reading the committed manifest back out of
Azure DevOps. The guide now says this explicitly, since the reviewer is
unlikely to be the last person to assume otherwise.

Refs #288

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s carve-out

The docs insisted on leaving `username` out, which overstates what the code
does and framed a general rule as provider-specific.

Read back: nothing in CredentialFromSecretData mentions Azure DevOps. It
branches on `password`, and passes whatever `username` it found straight
through. So all three shapes work -- `password` alone, `username: ""` with a
password, and a real username with a password -- and only a username WITHOUT a
password is an error.

Measured against a real ADO repository, `CheckRepo` is accepted with the
username set to "", "pat", "anything-at-all", and the account's own name. ADO
genuinely ignores it, so "do not add a username" was advice for a problem that
does not exist.

configuration.md now states the rule where the auth-keys table is, since that
table said HTTP basic needs `username` + `password` and thereby contradicted
it: the password is what selects basic auth, the username is optional, and an
empty value is the same as an absent key to the credential reader. That last
point is the only genuinely surprising part, and it is a property of
firstSecretValue rather than of Azure DevOps.

The getting-started guide drops the warning and says the username is optional
and ignored, which is both shorter and true.

A test now covers the case that had none: a supplied username must survive
into the credential. The three ADO-shaped cases were already covered; the
ordinary one was not, which is how the docs drifted from the code unnoticed.

Refs #288

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/facts/azure-devops-multi-ack-requirement.md`:
- Around line 130-133: Update the simulator note to describe the check as
matching the raw “multi_ack” substring rather than a prefix, including the
multi_ack_detailed form. Keep the existing explanation of the simulator’s
stricter behavior and its acceptance of the bare-want shape unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7fc86b96-af1f-4476-bcbb-67d05fe39941

📥 Commits

Reviewing files that changed from the base of the PR and between f7b2c39 and a24a1cc.

📒 Files selected for processing (7)
  • docs/README.md
  • docs/azure-devops-getting-started.md
  • docs/configuration.md
  • docs/facts/azure-devops-multi-ack-requirement.md
  • docs/security-model.md
  • internal/git/ado_live_test.go
  • internal/git/credentials_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • docs/configuration.md
  • docs/azure-devops-getting-started.md
  • internal/git/credentials_test.go

Comment on lines +130 to +133
The simulator is deliberately stricter than ADO: it rejects any `upload-pack` POST carrying neither
`multi_ack` nor `multi_ack_detailed` — its check matches the `multi_ack` prefix, so either satisfies
it, exactly as ADO does — including the bare-`want` shape ADO accepts. That difference does not matter for what it
gates, and the strictness is what keeps it simple.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file existence / size =="
ls -l docs/facts/azure-devops-multi-ack-requirement.md || true
wc -l docs/facts/azure-devops-multi-ack-requirement.md || true

echo "== relevant section =="
sed -n '1,180p' docs/facts/azure-devops-multi-ack-requirement.md | cat -n

echo "== search simulator implementation mentions =="
rg -n --hidden --glob '!vendor/**' --glob '!dist/**' --glob '!build/**' 'multi_ack|upload-pack|Contains' .

Repository: ConfigButler/gitops-reverser

Length of output: 50386


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
p = Path("docs/facts/azure-devops-multi-ack-requirement.md")
text = p.read_text() if p.exists() else ""
print("contains 'prefix' or 'prefix' in relevant document:")
for i, line in enumerate(text.splitlines(), 1):
    if "prefix" in line.lower() or "substring" in line.lower() or "bytes.Contains" in line:
        print(f"{i}: {line}")

# Behavioral probe for Go semantics implied by bytes.Contains:
# raw substring would treat any string containing "multi_ack" as matching,
# including false-positive keys like "nomulti_ack", which a token-aware prefix/prefix-list check should not.
samples = [
    "multi_ack",
    "multi_ack_detailed",
    "multi_ack_detailed_and_something",
    "nomulti_ack",
    "multi_ack_in_middle",
    "wantedmulti_ack",
]
needle = "multi_ack"
for s in samples:
    print(f"{s!r}: contains={needle in s}, plausible_single_token_boundary={s.count('_') == 1 and s.startswith(needle) and s[9:10] in (' ','\t','>', '<', '/', '?', '&') and (len(s)==9 or s[9:][0].isupper())}")
PY

Repository: ConfigButler/gitops-reverser

Length of output: 677


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== multi_ack simulator / ADO proxy candidates =="
rg -n --hidden --glob '!vendor/**' --glob '!dist/**' --glob '!build/**' \
  '(multi_ack_detailed|multi_ack|UploadPack|git-upload-pack|400|TF401041|upload-pack)' internal test pkg cmd docs | \
  head -n 200

echo "== focused filenames =="
git ls-files | rg '(^|/)(ado_multiack|multiack|multi-ack|multi_ack).*\.go$|ado|multi_ack'

Repository: ConfigButler/gitops-reverser

Length of output: 50384


🏁 Script executed:

#!/bin/bash
set -euo pipefail
python3 - <<'PY'
needle = "multi_ack"
samples = [
    "multi_ack",
    "multi_ack_detailed",
    "multi_ack_detailed_and_something",
    "nomulti_ack",
    "multi_ack_in_middle",
    "wantedmulti_ack",
]
for s in samples:
    print(f"{s!r}: contains={needle in s}")
PY

Repository: ConfigButler/gitops-reverser

Length of output: 378


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== go.mod go version =="
sed -n '1,40p' go.mod 2>/dev/null || true

echo "== ADO multi_ack files by name =="
git ls-files | rg -in 'multi[ _-]?ack|mack|ado.*multi|multi.*ado' || true

echo "== focused exact matches for multi_ack_detailed literal =="
rg -n --hidden --glob '!vendor/**' --glob '!dist/**' --glob '!build/**' 'multi_ack_detailed|multi_ack' internal cmd test docs --glob '*.go' --glob '!*.svg' --glob '!*.cast' | head -n 200

Repository: ConfigButler/gitops-reverser

Length of output: 5527


Replace “prefix” with substring in the simulator note.

The simulator contract uses bytes.Contains(body, []byte("multi_ack")), including multi_ack_detailed, so calling this a prefix match overstates the check’s precision. Document the raw multi_ack substring behavior or tighten the simulator.

🧰 Tools
🪛 LanguageTool

[style] ~132-~132: To elevate your writing, try using an alternative expression here.
Context: ...ant` shape ADO accepts. That difference does not matter for what it gates, and the strictness i...

(MATTERS_RELEVANT)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/facts/azure-devops-multi-ack-requirement.md` around lines 130 - 133,
Update the simulator note to describe the check as matching the raw “multi_ack”
substring rather than a prefix, including the multi_ack_detailed form. Keep the
existing explanation of the simulator’s stricter behavior and its acceptance of
the bare-want shape unchanged.

@sunib sunib changed the title feat(git): go-git v6, so Azure DevOps repositories can be fetched at all feat(git): go-git v6, so Azure DevOps is now also supported Jul 30, 2026
@sunib
sunib merged commit b811706 into main Jul 30, 2026
19 checks passed
@sunib
sunib deleted the feat/go-git-v6 branch July 30, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] fetch fails with HTTP 400 against Azure DevOps repositories (multi_ack capability)

1 participant